Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[energidataservice] Provide work-around for currency issues #16085

Merged
merged 2 commits into from Dec 20, 2023

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Dec 19, 2023

This temporary work-around addresses some issues regarding currency support in openhab/openhab-core#3503 and #16070.

See #16070 (comment)

Issues:

  • State updates are dependent on user configuration. For example, it is not possible to update a channel with currency DKK if configured country is not Denmark (for LocaleBasedCurrencyProvider) or configured fixed base currency is not DKK (for FixedCurrencyProvider).
  • For the same reason, it is not possible to update channels with mixed currencies, thereby breaking the feature of fetching spot prices in EUR (while tariffs are still in DKK).

The provided work-around will ensure that Number items can always be updated, and Number:EnergyPrice as well, although it cannot be guaranteed to be in the correct currency/unit.

State Provider Configuration Result Work-around result
0.33126001 kr./kWh LocaleBasedCurrencyProvider Denmark 0.33126001 kr./kWh 0.33126001 kr./kWh
0.33126001 DKK/kWh LocaleBasedCurrencyProvider Denmark java.lang.IllegalArgumentException: Invalid Quantity value: 0.33126001 DKK/kWh 0.33126001 kr./kWh
0.33126001 kr./kWh LocaleBasedCurrencyProvider Sweden java.lang.IllegalArgumentException: Invalid Quantity value: 0.33126001 kr./kWh 0.33126001 SEK/kWh
0.33126001 kr./kWh FixedCurrencyProvider DKK 0.33126001 kr./kWh 0.33126001 kr./kWh
0.33126001 kr./kWh FixedCurrencyProvider EUR 0.33126001 €./kWh 0.33126001 €./kWh
0.33126001 DKK/kWh FixedCurrencyProvider DKK java.lang.IllegalArgumentException: Invalid Quantity value: 0.33126001 DKK/kWh 0.33126001 kr./kWh
0.044770 €/kWh FixedCurrencyProvider DKK java.lang.IllegalArgumentException: Invalid Quantity value: 0.044770 €/kWh 0.044770 kr./kWh
0.044770 €/kWh FixedCurrencyProvider EUR 0.044770 €/kWh 0.044770 €/kWh

@jlaur jlaur added bug An unexpected problem or unintended behavior of an add-on regression Regression that happened during the development of a release. Not shown on final release notes. labels Dec 19, 2023
@jlaur jlaur requested a review from J-N-K December 19, 2023 22:21
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the energidataservice-currency-workaround branch from 75e459c to f1b67ea Compare December 19, 2023 22:42
return new QuantityType<>(price + " " + currency.getSymbol() + "/kWh");
} catch (IllegalArgumentException e) {
logger.trace("Unable to create QuantityType, falling back to DecimalType", e);
return new DecimalType(price);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also fall back to CurrencyUnits.BASE_CURRENCY.getSymbol(), which is always available. Or check if the desired unit is available by CurrencyUnits.getInstance().getUnit(String s).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm now verifying the unit before trying to instantiate QuantityType from it.

Additionally I will now fallback to currency code if there is no symbol for the currency, although my tests suggest that currency code as unit is not supported. Also I'm now using getSymbol() on the returned Unit rather than Currency.getSymbol(), because that is display unit for the Locale and thus prone to failure (e.g. "$" vs. "US$").

I don't think using base currency is such a good idea, because that would mean explicitly providing a wrong unit. The end result is currently the same though.

Copy link
Contributor Author

@jlaur jlaur Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this only as a work-around to ensure reliable state updates in 4.1, would you agree? And do you see anything in all this that is worth creating an issue for in core?

We currently have the limitation that the UoM system know only one currency. When this will change, I would somehow expect:

  • Always being able to update state for known currencies, even if there is no currency exchange service provided/installed - without risking IllegalArgumentException being thrown. With known currencies, all I mean all defined by ISO 4217.
  • In this case IMHO it would still be better to keep the source currency unit rather than overwriting by user configured currency unit without converting the amount. I know this contradicts 4.0 UoM because we cannot enforce "target unit", and I don't have a good proposal for tackling that. It's different, because we can always expect to be able to convert °C to °F, but we can't always expect to be able to convert DKK to EUR. And we cannot make the assumption that exchange rate is 100, because then prices will be plain misleading.
  • Being able to update state with currency code as unit rather than symbol. When all currencies will become available and we are able to convert between them, symbols can become ambiguous as previously discussed.

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@J-N-K J-N-K added this to the 4.1 milestone Dec 20, 2023
@J-N-K J-N-K merged commit 9872ca7 into openhab:main Dec 20, 2023
3 checks passed
@jlaur jlaur deleted the energidataservice-currency-workaround branch December 20, 2023 10:25
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/using-the-new-currency-units-of-measurement-in-4-1/152338/17

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/using-the-new-currency-units-of-measurement-in-4-1/152338/22

austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
…16085)

* Provide work-around for currency issues
* Verify unit before trying to instantiate QuantityType

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on regression Regression that happened during the development of a release. Not shown on final release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants